check the layer media type before flashing#9
check the layer media type before flashing#9bennyz wants to merge 3 commits intojumpstarter-dev:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes enhance OCI manifest layer selection by introducing an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/fls/oci/manifest.rs (1)
67-69: Add direct tests for the new selection rules.A couple of focused unit tests for (1)
artifact_typeselection, (2) single non-disk layer rejection, and (3) the no-match error would cover the new branches inget_single_layer(). The current tests in this file only exercise parsing.Also applies to: 149-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/manifest.rs` around lines 67 - 69, Add three focused unit tests for Manifest::get_single_layer covering the new selection branches: (1) create a manifest with artifact_type set and layers such that the correct layer is chosen then assert get_single_layer returns that layer, (2) create a manifest with a single layer whose mediaType is not a disk type and assert get_single_layer returns the expected rejection/error, and (3) create a manifest with no matching layers and assert get_single_layer returns the no-match error. Locate the Manifest struct and the get_single_layer method to construct test Manifests and expected Layer descriptors, use the same error types returned by get_single_layer for assertions, and add these tests adjacent to the existing parsing tests so they exercise lines handling artifact_type and the layer-selection branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fls/oci/manifest.rs`:
- Around line 145-163: The early-return and artifact_type match currently accept
any layer regardless of media_type; update the selection logic in manifest.rs so
every successful path validates the layer.media_type against the allowed
flashable disk types (e.g., check starts_with("application/vnd.automotive.disk")
or an equivalent allowed list) before returning; specifically, when
m.layers.len() == 1 verify m.layers[0].media_type is flashable, when matching
via m.artifact_type ensure the found layer also has a flashable media_type, and
keep the existing fallback that finds the first layer whose media_type
starts_with the disk prefix.
---
Nitpick comments:
In `@src/fls/oci/manifest.rs`:
- Around line 67-69: Add three focused unit tests for Manifest::get_single_layer
covering the new selection branches: (1) create a manifest with artifact_type
set and layers such that the correct layer is chosen then assert
get_single_layer returns that layer, (2) create a manifest with a single layer
whose mediaType is not a disk type and assert get_single_layer returns the
expected rejection/error, and (3) create a manifest with no matching layers and
assert get_single_layer returns the no-match error. Locate the Manifest struct
and the get_single_layer method to construct test Manifests and expected Layer
descriptors, use the same error types returned by get_single_layer for
assertions, and add these tests adjacent to the existing parsing tests so they
exercise lines handling artifact_type and the layer-selection branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55bbc641-f85a-4511-a0be-17bb83c99ad0
📒 Files selected for processing (1)
src/fls/oci/manifest.rs
| if m.layers.len() == 1 { | ||
| return Ok(&m.layers[0]); | ||
| } | ||
|
|
||
| // If artifactType is set, find the layer matching it | ||
| if let Some(ref artifact_type) = m.artifact_type { | ||
| if let Some(layer) = m.layers.iter().find(|l| l.media_type == *artifact_type) { | ||
| return Ok(layer); | ||
| } | ||
| } | ||
|
|
||
| // Fall back to the first disk image layer | ||
| if let Some(layer) = m | ||
| .layers | ||
| .iter() | ||
| .find(|l| l.media_type.starts_with("application/vnd.automotive.disk")) | ||
| { | ||
| return Ok(layer); | ||
| } |
There was a problem hiding this comment.
Validate all successful paths against the flashable media types.
Line 145 returns the only layer before any media-type check, and Lines 149-153 will also accept any artifact_type match. That still lets a non-disk blob through this path, which is exactly what this PR is trying to prevent.
Minimal fix
Manifest::Image(ref m) => {
+ let is_flashable = |layer: &Descriptor| {
+ layer.media_type.starts_with("application/vnd.automotive.disk")
+ };
+
if m.layers.is_empty() {
return Err("Manifest has no layers".to_string());
}
if m.layers.len() == 1 {
- return Ok(&m.layers[0]);
+ let layer = &m.layers[0];
+ return if is_flashable(layer) {
+ Ok(layer)
+ } else {
+ Err(format!("Layer media type '{}' is not flashable", layer.media_type))
+ };
}
// If artifactType is set, find the layer matching it
if let Some(ref artifact_type) = m.artifact_type {
- if let Some(layer) = m.layers.iter().find(|l| l.media_type == *artifact_type) {
+ if let Some(layer) = m.layers.iter().find(|l| {
+ l.media_type == *artifact_type && is_flashable(l)
+ }) {
return Ok(layer);
}
}
// Fall back to the first disk image layer
if let Some(layer) = m
.layers
.iter()
- .find(|l| l.media_type.starts_with("application/vnd.automotive.disk"))
+ .find(|l| is_flashable(l))
{
return Ok(layer);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fls/oci/manifest.rs` around lines 145 - 163, The early-return and
artifact_type match currently accept any layer regardless of media_type; update
the selection logic in manifest.rs so every successful path validates the
layer.media_type against the allowed flashable disk types (e.g., check
starts_with("application/vnd.automotive.disk") or an equivalent allowed list)
before returning; specifically, when m.layers.len() == 1 verify
m.layers[0].media_type is flashable, when matching via m.artifact_type ensure
the found layer also has a flashable media_type, and keep the existing fallback
that finds the first layer whose media_type starts_with the disk prefix.
There was a problem hiding this comment.
@mangelajo, thanks for the confirmation! The fix is straightforward — every successful return path in get_single_layer needs to validate the layer's media type against the flashable disk prefix. Here's the suggested update:
pub fn get_single_layer(&self) -> Result<&Descriptor, String> {
match self {
Manifest::Image(ref m) => {
let is_flashable = |layer: &Descriptor| {
layer.media_type.starts_with("application/vnd.automotive.disk")
};
if m.layers.is_empty() {
return Err("Manifest has no layers".to_string());
}
if m.layers.len() == 1 {
let layer = &m.layers[0];
return if is_flashable(layer) {
Ok(layer)
} else {
Err(format!(
"Layer media type '{}' is not a flashable disk image",
layer.media_type
))
};
}
// If artifactType is set, find a layer matching it that is also flashable
if let Some(ref artifact_type) = m.artifact_type {
if let Some(layer) = m.layers.iter().find(|l| {
l.media_type == *artifact_type && is_flashable(l)
}) {
return Ok(layer);
}
}
// Fall back to the first disk image layer
if let Some(layer) = m.layers.iter().find(|l| is_flashable(l)) {
return Ok(layer);
}
Err(format!(
"No disk image layer found among {} layers",
m.layers.len()
))
}
Manifest::Index(_) => Err(
"Cannot get layer from manifest index - need to resolve platform first".to_string(),
),
}
}Key changes vs the current PR:
- Single-layer path: now rejects the layer if its
media_typedoesn't start withapplication/vnd.automotive.disk. artifact_typepath: the closure additionally requiresis_flashable, so a matching artifact type alone is not enough.- Fallback path: already correct, just uses the shared
is_flashableclosure for consistency.
Would you like me to open this as a plan for code changes?
There was a problem hiding this comment.
Fixed in ab866a4. All three return paths in get_single_layer() now validate the media type using a shared is_flashable closure that checks for the application/vnd.automotive.disk prefix:
- Single layer path: now returns an error if the layer has a non-flashable media type
- artifact_type path: now requires both
media_type == artifact_typeANDis_flashable - Fallback path: uses the same
is_flashableclosure (functionally unchanged, just refactored for consistency)
Also added unit tests covering: single flashable layer acceptance, single non-disk layer rejection, artifact_type-based selection, and no-match error.
There was a problem hiding this comment.
Fixed — but with an important design adjustment: get_single_layer() is shared between the flash and extract paths. The extraction path (extract_files_from_oci_image_to_dir) legitimately needs to work with non-disk media types (e.g. tar+gzip layers), so adding flashable validation inside get_single_layer() broke the 5 oci_extract integration tests.
The fix instead:
- Keeps
get_single_layer()generic — single-layer manifests return the layer directly (callers decide what's valid) - Adds flashable validation in
flash_from_oci()— the flash-specific path now rejects layers that don't start withapplication/vnd.automotive.disk - Multi-layer selection still prefers
artifact_typematch, then falls back to the first disk image layer
Also fixed the &&layer clippy lint (needless borrow).
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| if let Some(layer) = m | ||
| .layers | ||
| .iter() | ||
| .find(|l| l.media_type.starts_with("application/vnd.automotive.disk")) |
There was a problem hiding this comment.
I would probably add some other more generic media types as well, so the oci artifacts could be reused in other areas. applcation/vnd.embedded.disk, or similar.
There was a problem hiding this comment.
@coderabbitai can you open a follow-up issue for this?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo jumpstarter-dev/fls \
--title "Support more generic media types for disk image layer selection" \
--body "## Follow-up: Support more generic media types for disk image layer selection
Currently, the fallback layer selection in \`get_single_layer()\` (in \`src/fls/oci/manifest.rs\`) only matches layers whose \`mediaType\` starts with \`application/vnd.automotive.disk\`.
To allow OCI artifacts to be reused in other areas (e.g. embedded systems beyond automotive), the selection logic should also consider more generic media types such as \`application/vnd.embedded.disk\` or similar conventions.
### Suggested improvement
Expand the media type matching in the fallback layer selection to include additional generic disk image media types, for example:
- \`application/vnd.embedded.disk\`
- Other relevant generic media types as appropriate
This would make \`fls\` more broadly applicable beyond automotive use cases.
---
Raised by `@mangelajo` in `#9`: https://github.com/jumpstarter-dev/fls/pull/9#discussion_r3043976233" \
--assignee mangelajo
Length of output: 48
@mangelajo, I've opened a follow-up issue for this: #12 – Support more generic media types for disk image layer selection.
It covers expanding the fallback layer selection in get_single_layer() to support more generic media types (e.g., application/vnd.embedded.disk) beyond the current application/vnd.automotive.disk prefix, making fls more broadly applicable beyond automotive use cases.
|
@ambient-code please handle existing comments. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/fls/oci/manifest.rs (1)
340-365: Make theartifact_typetest proveartifact_typeprecedence.As written, this still passes if the
artifact_typebranch is removed, because the fallback path also returns the same layer. Use two flashable layers with different media types so the test verifies the explicitartifactTypematch wins, and add a regression case for a non-flashableartifactType.🧪 Suggested test shape
fn test_artifact_type_selection() { let json = r#"{ "schemaVersion": 2, - "artifactType": "application/vnd.automotive.disk.raw", + "artifactType": "application/vnd.automotive.disk.qcow2", "config": { "mediaType": "application/vnd.oci.image.config.v1+json", "digest": "sha256:config123", "size": 100 }, "layers": [ { - "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip", - "digest": "sha256:layer1", + "mediaType": "application/vnd.automotive.disk.raw", + "digest": "sha256:disk-raw", "size": 1000 }, { - "mediaType": "application/vnd.automotive.disk.raw", - "digest": "sha256:disk1", + "mediaType": "application/vnd.automotive.disk.qcow2", + "digest": "sha256:disk-qcow2", "size": 9999 } ] }"#; let manifest = Manifest::parse(json.as_bytes(), None).unwrap(); let layer = manifest.get_single_layer().unwrap(); - assert_eq!(layer.digest, "sha256:disk1"); + assert_eq!(layer.digest, "sha256:disk-qcow2"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/manifest.rs` around lines 340 - 365, The test test_artifact_type_selection currently doesn't prove artifactType precedence because the fallback path selects the same layer; update the test to include two flashable layers with distinct mediaTypes (e.g., one with "application/vnd.automotive.disk.raw" and another with a different flashable mediaType) and set artifactType in the manifest JSON to match only one of them so Manifest::parse(... ) and manifest.get_single_layer() must pick the layer whose mediaType equals artifactType; also add a regression case where artifactType is a non-flashable mediaType to assert that the fallback selection logic is used (or an error is returned) to ensure the artifact_type branch is actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/fls/oci/manifest.rs`:
- Around line 340-365: The test test_artifact_type_selection currently doesn't
prove artifactType precedence because the fallback path selects the same layer;
update the test to include two flashable layers with distinct mediaTypes (e.g.,
one with "application/vnd.automotive.disk.raw" and another with a different
flashable mediaType) and set artifactType in the manifest JSON to match only one
of them so Manifest::parse(... ) and manifest.get_single_layer() must pick the
layer whose mediaType equals artifactType; also add a regression case where
artifactType is a non-flashable mediaType to assert that the fallback selection
logic is used (or an error is returned) to ensure the artifact_type branch is
actually exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef4d7957-3342-4ec6-ac48-75165781075e
📒 Files selected for processing (1)
src/fls/oci/manifest.rs
ambient-code
bot
force-pushed
the
fix-layer-selection
branch
from
ab866a4 to
5dfbeb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fls/oci/manifest.rs`:
- Around line 142-168: Update the stale doc comment for the method that selects
a disk image layer (the code using m.layers, m.artifact_type and
layer.media_type) to describe the new multi-layer selection rules: if only one
layer exists return it; otherwise, if m.artifact_type is set, return the layer
whose media_type equals the artifact_type; otherwise, fall back to the first
layer whose media_type starts with "application/vnd.automotive.disk"; if none
match, return an error. Mention these exact selection steps and the possible
error outcome so the doc matches the implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a6f08f4-40ad-415c-9efe-89d42e815fd5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.tomlsrc/fls/decompress.rssrc/fls/download_error.rssrc/fls/fastboot.rssrc/fls/from_url.rssrc/fls/mod.rssrc/fls/oci/from_oci.rssrc/fls/oci/manifest.rssrc/fls/oci/registry.rssrc/fls/options.rssrc/fls/stream_utils.rssrc/lib.rssrc/main.rstests/oci_extract.rs
✅ Files skipped from review due to trivial changes (3)
- Cargo.toml
- src/fls/mod.rs
- src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fls/oci/from_oci.rs
👮 Files not reviewed due to content moderation or server errors (8)
- src/fls/decompress.rs
- src/fls/fastboot.rs
- src/main.rs
- src/fls/options.rs
- src/fls/oci/registry.rs
- src/fls/from_url.rs
- tests/oci_extract.rs
- src/fls/download_error.rs
To avoid flashing non disk layers we need to check the media type Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
Address review feedback: add flashable media type validation (application/vnd.automotive.disk.*) in the flash_from_oci function, which is the correct place for flash-specific checks. Keep get_single_layer() generic since it is also used by the extraction path where any media type is valid. For multi-layer manifests, get_single_layer() prefers artifact_type matches and falls back to the first disk image layer. Add unit tests for: artifact_type selection, single layer (any type) returned, single flashable layer acceptance, and no-match error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bennyz
force-pushed
the
fix-layer-selection
branch
from
3c0afcb to
d9de095
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/fls/oci/manifest.rs (1)
136-138:⚠️ Potential issue | 🟡 MinorUpdate stale
get_single_layer()doc contract.Line 137 still says multiple layers are an error, but current logic returns an
artifact_typematch or first disk layer for multi-layer manifests. Please align the doc with actual selection behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/manifest.rs` around lines 136 - 138, The doc for get_single_layer() is stale: it claims multiple layers are an error, but the implementation actually matches artifact_type or selects the first disk layer for multi-layer manifests; update the function comment above pub fn get_single_layer(&self) -> Result<&Descriptor, String> to reflect the real contract (describe that it returns the matched artifact_type layer when present, otherwise the first disk layer, and only errors when no layers exist or no suitable layer can be found), so the documentation matches the behavior of get_single_layer.
🧹 Nitpick comments (1)
src/fls/oci/manifest.rs (1)
157-160: Extract disk media prefix to a shared constant.The automotive disk prefix is hard-coded here and also used in flash-path validation (
src/fls/oci/from_oci.rs:1815-1825). A shared constant would reduce drift risk.Proposed refactor
pub mod media_types { @@ pub const DOCKER_LAYER: &str = "application/vnd.docker.image.rootfs.diff.tar.gzip"; + pub const AUTOMOTIVE_DISK_PREFIX: &str = "application/vnd.automotive.disk"; } @@ - .find(|l| l.media_type.starts_with("application/vnd.automotive.disk")) + .find(|l| l.media_type.starts_with(media_types::AUTOMOTIVE_DISK_PREFIX))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fls/oci/manifest.rs` around lines 157 - 160, Extract the hard-coded automotive disk media prefix ("application/vnd.automotive.disk" currently used in manifest scanning and in flash-path validation) into a single shared constant and replace the literal occurrences. Define a public constant (e.g., AUTOMOTIVE_DISK_MEDIA_PREFIX) in a common module or at the top of src/fls/oci/manifest.rs (or a central constants module used by src/fls/oci/from_oci.rs), then update the .media_type.starts_with check in the code that iterates m.layers and the corresponding validation in from_oci.rs to use that constant instead of the string literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/fls/oci/manifest.rs`:
- Around line 136-138: The doc for get_single_layer() is stale: it claims
multiple layers are an error, but the implementation actually matches
artifact_type or selects the first disk layer for multi-layer manifests; update
the function comment above pub fn get_single_layer(&self) -> Result<&Descriptor,
String> to reflect the real contract (describe that it returns the matched
artifact_type layer when present, otherwise the first disk layer, and only
errors when no layers exist or no suitable layer can be found), so the
documentation matches the behavior of get_single_layer.
---
Nitpick comments:
In `@src/fls/oci/manifest.rs`:
- Around line 157-160: Extract the hard-coded automotive disk media prefix
("application/vnd.automotive.disk" currently used in manifest scanning and in
flash-path validation) into a single shared constant and replace the literal
occurrences. Define a public constant (e.g., AUTOMOTIVE_DISK_MEDIA_PREFIX) in a
common module or at the top of src/fls/oci/manifest.rs (or a central constants
module used by src/fls/oci/from_oci.rs), then update the .media_type.starts_with
check in the code that iterates m.layers and the corresponding validation in
from_oci.rs to use that constant instead of the string literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2f99286-340e-4131-a0c6-dc21f5e97890
📒 Files selected for processing (2)
src/fls/oci/from_oci.rssrc/fls/oci/manifest.rs
✅ Files skipped from review due to trivial changes (1)
- src/fls/oci/from_oci.rs
Use two distinct flashable layers so artifactType selection is distinguishable from the fallback path. Add a regression test for when artifactType matches no layer to verify fallback to disk media type. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
bennyz
force-pushed
the
fix-layer-selection
branch
from
d9de095 to
e3f85ba
Compare
2 participants
To avoid flashing non disk layers we need to check the media type